Rewrite metadata functions #457
Conversation
|
|
||
| <table> | ||
| <thead> | ||
| <tr><th>Element<th>Property name<th>TrustedType |
There was a problem hiding this comment.
I feel like this table needs a baseVal property for the SVGAnimatedString type somehow, not sure on the specifics of that SVG integration (I also am not sure that baseVal is accounted for in Chromium if it is required in this list).
lukewarlow
force-pushed
the
metadata-rewrite
branch
2 times, most recently
from
436bc9e to
862c806
Compare
lukewarlow
force-pushed
the
metadata-rewrite
branch
from
862c806 to
6362f3a
Compare
|
@mbrodesser-Igalia do you think you'd be able to give this a review? |
If this could wait until I actually start implementing it in Gecko, that might be most effective. |
Thorough reviews are time-consuming. |
lukewarlow
force-pushed
the
metadata-rewrite
branch
2 times, most recently
from
d2d9227 to
648f6b2
Compare
lukewarlow
force-pushed
the
metadata-rewrite
branch
from
3d67bea to
b2dfa87
Compare
lukewarlow
force-pushed
the
metadata-rewrite
branch
2 times, most recently
from
7fa73e4 to
123ce3a
Compare
| To <dfn abstract-op>Get Trusted Type data for attribute</dfn> given |element|, |attribute|, |attributeNs|, perform the following steps: | ||
|
|
||
| 1. Let |data| be null. | ||
| 1. If |attributeNs| is null, and |attribute| is the name of an [=event handler content attribute=], then: |
There was a problem hiding this comment.
I'm not really sure how you envision this lookup to work. There are many event handler content attributes that are not applicable to elements. And some only apply to a specific element.
There was a problem hiding this comment.
There are many event handler content attributes
I don't fully understand what you mean by this? What event handler content attribute doesn't apply to an element?
Do you mean legacy ones no longer applicable or something like that?
And some only apply to a specific element.
So for the purposes of trusted types I don't think we mind being overly restrictive here, it seems fine to consider them independent of the element (this is the case in Chromium's implementation)?
For context the plan in WebKit is to use HTMLElement::eventNameForEventHandlerAttribute with a null check.
There was a problem hiding this comment.
onreadystatechange?
I'm not sure we have a specification concept for eventNameForEventHandlerAttribute.
There was a problem hiding this comment.
Chrome enforces the TT on that attribute for a div element for example, so that aspect is fine.
There was a problem hiding this comment.
Can you clarify how that is fine?
How about onloadend?
There was a problem hiding this comment.
To close the loop there's a discussion about just using on* pattern to block anything that resembles an event listener. WICG/sanitizer-api#226
There was a problem hiding this comment.
Given we can't use the on* pattern what is the next best steps here?
There was a problem hiding this comment.
Both Chrome and WebKit have a way to look up EventHandlers from IDL which gives us the coverage we need to ensure stuff is protected. Idk if that corresponds to the current spec text, or how we would spec that?
The key bit is that all sinks for a given browser are included in that browser, which the spec as written I think covers?
There was a problem hiding this comment.
I think we should have an actual list of event handler attributes we care about (those on elements), which would be a subset of all event handler attributes. That's also what the Sanitizer API would need.
I don't think we want to do something with event handler attributes that only have meaning on a non-element object.
There was a problem hiding this comment.
Have made a new issue to track this and added an inline issue in the spec referencing it. #520
- getAttributeType and getPropertyType now use lookup tables.
lukewarlow
force-pushed
the
metadata-rewrite
branch
from
37e5d68 to
54bc99c
Compare
|
For the sake of keeping the spec moving I've made two new issues to track the remaining questions here and am going to get this merged in so we can remove the last StringContext attribute mentions. The whole spec needs an editorial run through and any issues here can be addressed as part of that or smaller follow ups. |
SHA: b7cb119 Reason: push, by lukewarlow Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: b7cb119 Reason: push, by lukewarlow Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #456 and #423, and #474
Preview | Diff